Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constify more of alloc::Layout #67494

Merged
merged 1 commit into from
Jan 12, 2020
Merged

Constify more of alloc::Layout #67494

merged 1 commit into from
Jan 12, 2020

Conversation

lukaslueg
Copy link
Contributor

Making more methods of alloc::Layout const would allow us to compute alignment/size information for arbitrary (sized) types at compile-time, including placing the information in associated constants. While mem::size_of and mem::align_of are already const and Layout is solely based on those, there is no guarantee in the implementation that a const derived from these functions will be exactly the same as what Layout uses and is subsequently used in a call to alloc::alloc. Constifying Layout makes this possible.

First contribution to core, excuse my ignorance.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2019
@Centril
Copy link
Contributor

Centril commented Dec 21, 2019

r? @oli-obk

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file a tracking issue for this constification and use the id instead of "0"

src/libcore/alloc.rs Outdated Show resolved Hide resolved
src/libcore/alloc.rs Outdated Show resolved Hide resolved
src/libcore/alloc.rs Outdated Show resolved Hide resolved
@lukaslueg
Copy link
Contributor Author

I split the consttransmogrification of Result into a separate feature, I hope this was ok.

pub fn align_to(&self, align: usize) -> Result<Self, LayoutErr> {
Layout::from_size_align(self.size(), cmp::max(self.align(), align))
pub const fn align_to(&self, align: usize) -> Result<Self, LayoutErr> {
let align = if self.align() >= align { self.align() } else { align };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make cmp::max const fn, too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I file a separate tracking issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I missed this in the first pass

the PR lgtm after this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just realized: std::cmp::{max; min} are defined as pub fn max<T>(v1: T, v2: T) -> T where T: Ord. There is no way to require an impl of Ord to be const?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no that's not possible. So leave align_to as not const fn for now, or is it super necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to me, no. I actually don't see this one-liner as a problem: The type is always going to be usize, so even if it never gets changed back to cmp::max(), it's totally obvious what it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's totally obvious what it does.

I agree, but

  • the line of what is obvious and what not is very subjective, if at some point we have to start arguing about it, it seems good to start out conservative if there's no strong desire for the main change
  • the original author must have thought that cmp::max is even more obvious (and I think this, too). So making code less obvious just to make it const without a strong reason to do so seems not like a good strategy.

Thanks for changing it back

Copy link

@rustonaut rustonaut Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a const fn align_max(align1: usize, align2: usize) -> usize helper function to replace cmpp::max()?

(Not in this PR, it's already merged)

@oli-obk
Copy link
Contributor

oli-obk commented Dec 22, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 22, 2019

📌 Commit b56e0e2126eaee3675805485169338d7736dd5bd has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Dec 22, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2019
@lukaslueg
Copy link
Contributor Author

I've just seen this will conflict with #66254 which does constification for just new. You may want to r- ?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 22, 2019

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 22, 2019
@bors
Copy link
Contributor

bors commented Dec 23, 2019

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

This was referenced Dec 25, 2019
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 31, 2019
Constify Result

r? @oli-obk

This is just the `Result`-part of rust-lang#67494 which I'll resubmit once rust-lang#66254 has landed.
Centril added a commit to Centril/rust that referenced this pull request Dec 31, 2019
Constify Result

r? @oli-obk

This is just the `Result`-part of rust-lang#67494 which I'll resubmit once rust-lang#66254 has landed.
@lukaslueg
Copy link
Contributor Author

Force-pushed a commit that wont conflict with #66254

@CAD97
Copy link
Contributor

CAD97 commented Jan 9, 2020

wont conflict with #66254

Actually, since we both (necessarily) touch size_align to make it const. You could rebase on top of #66254 or just wait for it to get merged.

@lukaslueg
Copy link
Contributor Author

rebased again

@bors
Copy link
Contributor

bors commented Jan 11, 2020

☔ The latest upstream changes (presumably #68126) made this pull request unmergeable. Please resolve the merge conflicts.

@lukaslueg
Copy link
Contributor Author

rebased again

@oli-obk
Copy link
Contributor

oli-obk commented Jan 11, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2020

📌 Commit c5a9a14 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2020
@bors
Copy link
Contributor

bors commented Jan 12, 2020

⌛ Testing commit c5a9a14 with merge eb9af48328fc6dee800ed3fe2f55551322a09cd6...

Centril added a commit to Centril/rust that referenced this pull request Jan 12, 2020
Constify more of alloc::Layout

Making more methods of `alloc::Layout` const would allow us to compute alignment/size information for arbitrary (sized) types at compile-time, including placing the information in associated constants. While `mem::size_of` and `mem::align_of` are already const and `Layout` is solely based on those, there is no guarantee in the implementation that a const derived from these functions will be exactly the same as what `Layout` uses and is subsequently used in a call to `alloc::alloc`. Constifying `Layout` makes this possible.

First contribution to core, excuse my ignorance.
@Centril
Copy link
Contributor

Centril commented Jan 12, 2020

@bors retry rolled up.

bors added a commit that referenced this pull request Jan 12, 2020
Rollup of 6 pull requests

Successful merges:

 - #67494 (Constify more of alloc::Layout)
 - #67867 (Correctly check for opaque types in `assoc_ty_def`)
 - #67948 (Galloping search for binary_search_util)
 - #68045 (Move more of `rustc::lint` into `rustc_lint`)
 - #68089 (Unstabilize `Vec::remove_item`)
 - #68108 (Add suggestions when encountering chained comparisons)

Failed merges:

r? @ghost
@bors bors merged commit c5a9a14 into rust-lang:master Jan 12, 2020
@lukaslueg lukaslueg deleted the const_alloc branch January 12, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants